src/app/loans/edit-loans-account/edit-loans-account.component.ts (2)

141-145: Include feeOnMonthDay when present to avoid losing Annual Fee dates

The step component sets feeOnMonthDay for 'Annual Fee'. The payload currently only sends dueDate, which may drop that info.

-      charges: Array.from(uniqueCharges.values()).map((charge: any) => ({
-        chargeId: charge.id || charge.chargeId,
-        amount: charge.amount,
-        dueDate: charge.dueDate && this.dateUtils.formatDate(charge.dueDate, dateFormat)
-      })),
+      charges: Array.from(uniqueCharges.values()).map((charge: any) => {
+        const payload: any = {
+          chargeId: charge.id ?? charge.chargeId,
+          amount: charge.amount
+        };
+        if (charge.dueDate) {
+          payload.dueDate = this.dateUtils.formatDate(charge.dueDate, dateFormat);
+        }
+        if (charge.feeOnMonthDay) {
+          payload.feeOnMonthDay = this.dateUtils.formatDate(charge.feeOnMonthDay, dateFormat);
+        }
+        if (charge.feeInterval != null) {
+          payload.feeInterval = charge.feeInterval;
+        }
+        return payload;
+      }),

Please confirm Fineract expects feeOnMonthDay/feeInterval for your charge types in PUT /v1/loans. If not, we can pare this back. Based on learnings.

---

132-137: Avoid double-dedup: rely on a single, shared helper to keep behavior consistent

We dedup in the step getter and again here. Prefer one source of truth (e.g., a shared util), and keep a server-side guard (filter invalids) here.

I can extract a shared function (e.g., loans-charge-utils.ts) and update both call sites. Want me to draft it?

Also applies to: 141-145

src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (3)

165-176: Prefer existing loan charges on modification; current order may transiently show product charges

OnChanges runs before OnInit; with empty chargesDataSource, the first branch loads product charges even in edit mode, then OnInit overwrites with existing charges. This causes redundant work and potential UI flicker.

-      const isModification = this.loanId != null;
-      if (
-        this.loansAccountProductTemplate.charges &&
-        this.loansAccountProductTemplate.charges.length > 0 &&
-        this.chargesDataSource.length === 0
-      ) {
-        this.chargesDataSource =
-          this.loansAccountProductTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || [];
-      } else if (isModification && this.loansAccountTemplate && this.loansAccountTemplate.charges) {
-        this.chargesDataSource =
-          this.loansAccountTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || [];
-      }
+      const isModification = this.loanId != null;
+      if (isModification && this.loansAccountTemplate?.charges?.length) {
+        this.chargesDataSource =
+          this.loansAccountTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || [];
+      } else if (this.chargesDataSource.length === 0 && this.loansAccountProductTemplate?.charges?.length) {
+        this.chargesDataSource =
+          this.loansAccountProductTemplate.charges.map((charge: any) => ({ ...charge, id: charge.chargeId })) || [];
+      }

Follow-up: consider moving init from ngOnInit to ngOnChanges and deleting the ngOnInit block that sets chargesDataSource to avoid duplication. Based on coding guidelines.

---

320-323: Confirm business rule: should duplicate chargeId entries ever be allowed?

If some charge types legitimately appear multiple times (e.g., specified due dates), hard dedup by id will drop them. If duplicates are only the bug scenario (WEB-117), proceed; otherwise, key by a composite (id + dueDate) when the chargeTimeType allows multiples.

I can implement conditional dedup (by id, or by id+date when charge.allowMultiple) if you confirm the rule.

---

141-146: *Add a trackBy function to the charges table’s ngFor
Define and reference a trackBy in loans-account-charges-step.component.html (e.g. <tr *ngFor="let charge of chargesDataSource; trackBy: trackByChargeId">) and add to the component:

trackByChargeId(_: number, charge: any) { 
  return charge.id; 
}

This ensures row identity is preserved after array updates, avoiding unnecessary DOM churn.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)

358-370: Deduplication key should fallback to id to match addCharge logic.

Line 362 uses only charge.chargeId without fallback. This will skip charges that have id but no chargeId, causing inconsistency with addCharge (line 201) which uses id as fallback.

Apply this diff to align with addCharge and handle both fields:

   private getUniqueCharges<T extends { id?: number | string; chargeId?: number | string }>(charges: T[]): T[] {
     const uniqueChargesMap = new Map<number | string, T>();
 
     for (const charge of charges ?? []) {
-      const chargeId = charge.chargeId;
+      const chargeId = charge.chargeId ?? charge.id;
       if (chargeId == null) {
         continue;
       }
       uniqueChargesMap.set(chargeId, charge);
     }
 
     return Array.from(uniqueChargesMap.values());
   }

This ensures charges with only id (or only chargeId) are not silently dropped during deduplication.

src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (3)

143-149: Redundant property assignments after spread.

The spread operator ...charge already includes id and chargeId, so explicitly setting them again has no effect unless the values are being transformed.

Apply this diff to simplify:

-    this.chargesDataSource =
-      this.loansAccountTemplate.charges.map((charge: any) => {
-        return {
-          ...charge,
-          id: charge.id,
-          chargeId: charge.chargeId
-        };
-      }) || [];
+    this.chargesDataSource = this.loansAccountTemplate.charges || [];

Or, if the intent is normalization (ensuring both fields exist), make it explicit:

     this.chargesDataSource =
       this.loansAccountTemplate.charges.map((charge: any) => {
         return {
           ...charge,
-          id: charge.id,
-          chargeId: charge.chargeId
+          chargeId: charge.chargeId ?? charge.id,
+          id: charge.id ?? charge.chargeId
         };
       }) || [];

---

199-203: Use nullish coalescing for safer fallback.

Line 201 uses || which treats falsy values (0, empty string) as missing. Use ?? to only fallback when null/undefined:

     const newCharge = {
       ...charge.value,
-      chargeId: charge.value.id || charge.value.chargeId
+      chargeId: charge.value.chargeId ?? charge.value.id
     };

Note: This makes chargeId the primary key (falling back to id), which aligns with getUniqueCharges at line 362 that keys by chargeId.

---

67-82: Consider defining a Charge interface for type safety.

The component uses any throughout for charge objects. Defining a typed interface would improve type safety and developer experience.

As per coding guidelines.

Example:

interface LoanCharge {
  id?: number;
  chargeId?: number;
  name?: string;
  amount?: number;
  dueDate?: string;
  feeInterval?: number;
  feeOnMonthDay?: string;
  chargeCalculationType?: any;
  chargeTimeType?: { value: string };
  // ... other properties
}

Then update:

-  chargesDataSource: {}[] = [];
+  chargesDataSource: LoanCharge[] = [];

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro